Skip to content
This repository has been archived by the owner on Jun 19, 2022. It is now read-only.

Add Deprecation Warnings to Topic and PullSubscription #980

Merged
merged 16 commits into from
May 9, 2020
Merged

Add Deprecation Warnings to Topic and PullSubscription #980

merged 16 commits into from
May 9, 2020

Conversation

Harwayne
Copy link
Contributor

@Harwayne Harwayne commented Apr 30, 2020 •
edited
Loading

Helps with #905.

Proposed Changes

  • Add deprecation warnings to every Topic and PullSubscription in the pubsub.cloud.google.com API group. 
    - lastTransitionTime: "2020-05-01T22:56:05Z"
  message: This Kind is deprecated and the CRD has been made 'internal'. This
    means, end-users should not create objects of this CRD directly. If you need
    a non-internal variant, then please let us know by commenting on https://github.com/google/knative-gcp/issues/905.
    Moreover, the object must be deleted before upgrading to 0.16 to avoid orphaning
    the Google Cloud Platform resources that were created on-behalf of this object,
    such as Pub/Sub Topics and Subscriptions.
  reason: willBeRemoved
  severity: Warning
  status: "True"
  type: Deprecated

Release Note

All `Topic` and `PullSubscription` objects in the `pubsub.cloud.google.com` API group will have deprecation warnings in their status. Those objects will no longer exist in the 0.16 release. All `Topic` and `PullSubscription` objects created by other reconcilers within `knative-gcp` will automatically migrate to the replacements without user interaction. **Migration may cause back logged events to be lost.**

@googlebot googlebot added the cla: yes (override cla status due to multiple authors bug) label Apr 30, 2020
@Harwayne
Copy link
Contributor Author

/test pull-google-knative-gcp-integration-tests

@Harwayne
Copy link
Contributor Author

Harwayne commented Apr 30, 2020 •
edited
Loading

/hold

Holding to allow @akashrv to confirm or alter the deprecation message.

akashrv
akashrv reviewed Apr 30, 2020
View reviewed changes
pkg/apis/pubsub/internal/deprecated_condition.go Outdated

const deprecationMessage = "This object is deprecated and should be deleted. There is no public " +
"replacement. If you want a public replacement, please comment on " +
"https://github.com/google/knative-gcp/issues/905. This object should be deleted before " +
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Content is correct. We could mellow down a bit. How about: "This object (or kind) is deprecated and the CRD is made 'internal'. This means, end-users should not create objects of this CRD directly. If you need a non-internal (or a public) variant, then please let us know by commenting on #905. Moreover, the object must be deleted before upgrading to 0.16 to avoid leaking (or orphaning) the Google Cloud Platform resources that were created on-behalf of this object, such as Pub/Sub topics and subscriptions."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, how about (almost identical to yours, but removed the parenthesis):

This Kind is deprecated and the CRD has been made 'internal'. This means, end-users should not create objects of this CRD directly. If you need a non-internal variant, then please let us know by commenting on https://github.com/google/knative-gcp/issues/905. Moreover, the object must be deleted before upgrading to 0.16 to avoid orphaning the Google Cloud Platform resources that were created on-behalf of this object, such as Pub/Sub Topics and Subscriptions.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/LGTM

@Harwayne
Copy link
Contributor Author

Harwayne commented May 1, 2020

/hold cancel

Cancelling hold now that the wording has been agreed upon.

@Harwayne
Copy link
Contributor Author

Harwayne commented May 4, 2020

Ping.

grantr
grantr approved these changes May 7, 2020
View reviewed changes
Copy link
Contributor

@grantr grantr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay in reviewing. Only minor nits, cancel the hold if desired.

/lgtm
/hold

pkg/apis/pubsub/internal/deprecated_condition.go Outdated Show resolved Hide resolved
pkg/apis/pubsub/internal/deprecated_condition.go Outdated Show resolved Hide resolved
grantr
grantr approved these changes May 8, 2020
View reviewed changes
Copy link
Contributor

@grantr grantr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/hold cancel

@Harwayne
Copy link
Contributor Author

Harwayne commented May 8, 2020

/retest

grantr
grantr approved these changes May 8, 2020
View reviewed changes
Copy link
Contributor

@grantr grantr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grantr, Harwayne

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Harwayne
Copy link
Contributor Author

Harwayne commented May 8, 2020

/retest

2 similar comments
@Harwayne
Copy link
Contributor Author

Harwayne commented May 8, 2020

/retest

@Harwayne
Copy link
Contributor Author

Harwayne commented May 9, 2020

/retest

@knative-metrics-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-google-knative-gcp-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pubsub/internal/deprecated_condition.go Do not exist 100.0%
pkg/apis/pubsub/v1alpha1/deprecated_condition.go Do not exist 0.0%
pkg/apis/pubsub/v1beta1/deprecated_condition.go Do not exist 0.0%
pkg/reconciler/pubsub/topic/topic.go 69.6% 69.9% 0.3

@knative-prow-robot knative-prow-robot merged commit 5cf7909 into google:master May 9, 2020
@Harwayne Harwayne deleted the 905-deprecation-warnings branch May 11, 2020 16:40
@knative-prow-robot
Copy link
Contributor

@Harwayne: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-google-knative-gcp-build-tests d275fae link /test pull-google-knative-gcp-build-tests
pull-google-knative-gcp-go-coverage 3693559 link /test pull-google-knative-gcp-go-coverage

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@akashrv akashrv akashrv left review comments

@grantr grantr grantr approved these changes

@capri-xiyue capri-xiyue Awaiting requested review from capri-xiyue

@ian-mi ian-mi Awaiting requested review from ian-mi

Assignees

@grantr grantr

@akashrv akashrv

Labels
approved cla: yes (override cla status due to multiple authors bug) lgtm size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Harwayne @knative-prow-robot @knative-metrics-robot @grantr @akashrv @googlebot